Skip to content

fix(portal): P1 behaviour fixes from v0.2.0 audit (CORS deny-cache, validator merge, PUT→PATCH, disable preserves origins)#104

Merged
voyvodka merged 1 commit into
mainfrom
fix/portal-p1-behavior
May 11, 2026
Merged

fix(portal): P1 behaviour fixes from v0.2.0 audit (CORS deny-cache, validator merge, PUT→PATCH, disable preserves origins)#104
voyvodka merged 1 commit into
mainfrom
fix/portal-p1-behavior

Conversation

@voyvodka
Copy link
Copy Markdown
Owner

Summary

Tur 4 of the v0.2.0 portal audit follow-up. Closes the four P1 behaviour findings; previous turns already shipped P0 security (#101), test coverage (#102), and docs (#103).

What's fixed

1. PortalCorsMiddleware deny-cache (security hardening)
HandlePreflightAsync now caches both allow and deny outcomes via IMemoryCache for the same TTL as the per-app signing-key lookup (default 60s). Browsers don't cache rejected preflights, so before this change every OPTIONS from a disallowed origin re-scanned the portal-enabled app set + deserialized the JSON allowlist — a free DB hammer vector for any caller that knew the portal prefix. Cache key is lowercased-origin scoped (RFC 6454 §4 case-insensitive).

  • New regression test Preflight_Deny_Decision_Is_Cached_Within_Ttl mutates the DB to allow the origin after a 403 and asserts the second call still 403s within the TTL window.

2. EndpointValidationRules helper consolidation (drift elimination)
New extension methods (EndpointUrlSyntax, EndpointDescription, EndpointTransformExpression, EndpointCustomHeaders, EndpointAllowedIpsCidrs, EndpointSecretOverride) become the single source of truth for the field-shape rules shared between the 4 admin endpoint validators (CreateEndpoint, UpdateEndpoint, DashboardCreateEndpoint, DashboardUpdateEndpoint) and the 2 portal endpoint validators (PortalCreateEndpoint, PortalUpdateEndpoint). Without this, tightening a rule on one surface silently leaves the other weaker — exactly the drift pattern the audit flagged. Async DNS host-safety check stays in each validator (the DependentRules + CustomAsync pattern needs the full property selector). Behaviour unchanged in this commit — the consolidation is a precondition for the next round of rule tightening.

3. PortalEndpointsController.Update[HttpPatch] (REST semantic fix — minor breaking)
The action's body semantics were always partial-replace (every field optional, only non-null fields applied) — that's PATCH, not PUT. Switching the verb aligns the wire surface with reality and stops misleading REST consumers that expect PUT to be full-replace. Route, request shape, response shape unchanged. The embeddable <EndpointManager /> component already issues PATCH. Two existing tests ported from PutAsJsonAsync to PatchAsync + JsonContent.Create.

4. DashboardPortalController.Disable preserves origins (operator UX fix)
Removed the line that nulled AllowedPortalOriginsJson on disable. Disable now revokes only the auth surface (PortalSigningKey, PortalRotatedAt) and keeps the operator-curated CORS allowlist so re-enable doesn't force re-curation. Explicit clear path remains: PUT /portal/origins with {origins: []}. Renamed test Disable_Clears_SigningKey_And_OriginsDisable_Clears_SigningKey_But_Preserves_Origins with assertion flip.

Test plan

  • dotnet build — 0 warnings, 0 errors.
  • dotnet test — 278 / 278 passing post-refactor (+1 new CORS test = 279 total after this PR).
  • CHANGELOG entries under Unreleased > Changed (3 fixes) + Security (CORS deny-cache).
  • CI green on PR.

Breaking change classification

Minor breaking — only #3 (PUTPATCH on /api/v1/portal/endpoints/{id}). Direct HTTP callers that were sending PUT will receive 405 Method Not Allowed. The shipped npm component (@webhookengine/endpoint-manager 0.1.0) already uses PATCH. Documented in CHANGELOG; no SemVer bump required because the portal is still pre-1.0 and the change happens within the same minor (v0.2.x → next).

Follow-ups (tracked, not in this PR)

  • Pre-B1 backlog: DOCKERHUB_TOKEN scope fix + ~50 lines redundant comment cleanup.
  • ADR write-ups for the v0.2.0 audit decisions (portal key plaintext + no rotation grace; CORS deny-cache TTL choice).

…alidator merge, PUT→PATCH, disable preserves origins)

Closes the four P1 behaviour findings from the v0.2.0 portal audit. Tur 1
(#101) shipped the P0 security fixes, Tur 2 (#102) shipped the test
coverage, Tur 3 (#103) shipped the docs; this Tur 4 closes the behaviour
delta.

1. PortalCorsMiddleware deny-cache.
   HandlePreflightAsync now caches both allow and deny outcomes via
   IMemoryCache for the same TTL as the per-app signing-key lookup
   (default 60s). Browsers don't cache rejected preflights, so every
   OPTIONS from a disallowed origin used to re-scan the portal-enabled
   app set + deserialize the JSON allowlist — a free DB hammer vector
   for any caller that knew the portal prefix. Cache key is
   lowercased-origin scoped (RFC 6454 §4 case-insensitive). New test
   Preflight_Deny_Decision_Is_Cached_Within_Ttl pins the behaviour by
   mutating the DB to allow the origin after a 403 and asserting the
   second call still 403s within the TTL window.

2. EndpointValidationRules helper consolidation.
   New extension methods (EndpointUrlSyntax, EndpointDescription,
   EndpointTransformExpression, EndpointCustomHeaders,
   EndpointAllowedIpsCidrs, EndpointSecretOverride) become the single
   source of truth for the field-shape rules shared between the 4
   admin endpoint validators and the 2 portal endpoint validators.
   Without this consolidation, tightening a rule on one surface
   silently leaves the other surface weaker — exactly the drift
   pattern the audit flagged. Async DNS host-safety check stays in
   each validator (DependentRules + CustomAsync needs the full
   property selector). Behaviour unchanged.

3. PortalEndpointsController.Update → [HttpPatch].
   The action's body semantics were always partial-replace (every
   field optional, only non-null fields applied) — that's PATCH, not
   PUT. Switching the verb aligns the wire surface with reality and
   stops misleading REST consumers that expect PUT to be full-replace.
   Route, request shape, response shape unchanged. Two existing tests
   ported from PutAsJsonAsync to PatchAsync + JsonContent.Create.

4. DashboardPortalController.Disable preserves origins.
   Removed the line that nulled AllowedPortalOriginsJson on disable.
   Disable now revokes only the auth surface (PortalSigningKey,
   PortalRotatedAt) and keeps the operator-curated CORS allowlist so
   re-enable doesn't force re-curation. Explicit clear path remains:
   PUT /portal/origins with {origins: []}. Renamed test
   Disable_Clears_SigningKey_And_Origins → Disable_Clears_SigningKey_But_Preserves_Origins
   with assertion flip.
@voyvodka voyvodka added bug Something is not working api API layer and endpoints infrastructure Docker, CI, deployment security Security-related issues labels May 11, 2026
@voyvodka voyvodka enabled auto-merge (squash) May 11, 2026 08:17
@voyvodka voyvodka merged commit b852901 into main May 11, 2026
7 checks passed
@voyvodka voyvodka deleted the fix/portal-p1-behavior branch May 11, 2026 08:20
voyvodka added a commit that referenced this pull request May 11, 2026
…e TTL) (#105)

Locks in two v0.2.0 audit decisions that were marked 'worth recording'
in the reviewer agent's punch list. Both are pure documentation —
behaviour was already shipped in PRs #101 and #104.

ADR-004 — Portal Signing Key Storage:
- Plaintext varchar(64) at rest, mirroring the existing SigningSecret
  column. No application-level encryption; relies on operator's
  Postgres deployment posture.
- Instant invalidation on rotate / disable. No grace window for
  overlapping old + new key validity.
- One-shot reveal on enable / rotate; subsequent reads return only
  portalEnabled bool + rotated-at + origins (never the key).
- Documents alternatives (pgcrypto, rotation grace, external KMS)
  and revisit triggers (compliance regimes, host-integration
  friction, key reuse beyond JWT verification).

ADR-005 — Portal CORS Preflight Deny-Cache TTL:
- 60s default (LookupCacheTtlSeconds reuse); cache both allow and
  deny outcomes; key is lowercased-origin scoped.
- No synchronous invalidation hook from PUT /portal/origins:
  cache key is origin-scoped, operator action is app-scoped —
  bookkeeping cost outweighs the <=60s staleness.
- Documents alternatives (per-app invalidation, shorter/longer TTL,
  negative-only cache) and revisit triggers (operator UX complaints,
  memory pressure via origin enumeration).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api API layer and endpoints bug Something is not working infrastructure Docker, CI, deployment security Security-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant